Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(next): Stop using NextUrl to avoid type conflict across version #62

Merged
merged 3 commits into from
Dec 15, 2023

Conversation

blaine-arcjet
Copy link
Contributor

Fixes #42

This removes the NextRequest["nextUrl"] usage from our code, which causes TypeScript compilation problems across different versions of Next.

This seems to be due to the NextUrl type using an internal symbol that TS sees as uinque across different versions. We should take care to remember that if we ever try to use symbols in the future.

I've also added a next.js 13 example that uses withArcjet to wrap some routes.

Copy link

socket-security bot commented Dec 14, 2023

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives Size Publisher
next 13.5.6 environment +10 1.11 GB vercel-release-bot

🚮 Removed packages: [email protected]

Copy link
Contributor

@davidmytton davidmytton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, although you may want to exclude this from Dependabot otherwise it's going to get annoying!

@blaine-arcjet
Copy link
Contributor Author

😬 Makes sense. Do you have any ideas on other ways to manage the dependencies of examples so we don't have conflicts if we need more changes? Perhaps we just need 1 example app each for next 13 and next 14 so it's just ignored once?

@davidmytton
Copy link
Contributor

Yeh I've been thinking about this. I've copied the idea from how Next.js does it at https://github.com/vercel/next.js/tree/canary/examples - they have more lightweight examples without the default files e.g. images, CSS, etc, so I'd like to do that. We should probably have just a single example app for each major version and also merge in app / pages dirs as well. Then we can more easily manage the ignore list.

I'd also like to have e2e tests on them as part of CI so we don't need to manually test every endpoint every time. But these are separate tasks! I suggest merging this and we can tackle these in a new issue.

This was referenced Dec 15, 2023
@blaine-arcjet
Copy link
Contributor Author

I don't see a good way to make dependabot ignore just next 13 inside that example. I think we'll need to do more work to separate the workspace members from the examples so dependabot can manage each example individually.

@blaine-arcjet blaine-arcjet merged commit 294540a into main Dec 15, 2023
7 checks passed
@blaine-arcjet blaine-arcjet deleted the phated/avoid-next-url branch December 15, 2023 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NextUrl type conflicts when the Next version of an app differs from our dependency
2 participants